Skip to content

Update compiler-rt to llvm-12 #14280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 25, 2021
Merged

Update compiler-rt to llvm-12 #14280

merged 1 commit into from
May 25, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 25, 2021

No description provided.

@sbc100 sbc100 requested a review from dschuff May 25, 2021 00:59
@sbc100 sbc100 force-pushed the update_compiler_rt branch from 8f6f87d to e3a88be Compare May 25, 2021 03:37
@sbc100 sbc100 requested a review from kripken May 25, 2021 03:41
@sbc100 sbc100 force-pushed the update_compiler_rt branch from e3a88be to 2045373 Compare May 25, 2021 05:41
@sbc100
Copy link
Collaborator Author

sbc100 commented May 25, 2021

To see the diff from upstream you can use this like:
llvm/llvm-project@llvmorg-12.0.0...emscripten-core:emscripten-libs-12.0.0

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the diff to upstream:

  • Is the diff in ubsan_checks.inc necessary?
  • The diff in ubsan_handlers.cpp looks odd - no ifdefing for emscripten, just a pure change to upstream?

thrd_detach: 'pthread_detach',
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be on a single line like the {{{ }}} stuff above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't make it work when the string is on the right of the ":" for some reason. It was giving me JS compiler errors. Plus I think this syntax is more readable in the end.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it works just several lines above in the diff? I don't understand. It's just adding a prefix to a string, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow it works in the key position but not in the value position. Oh maybe its because the keys are not in quotes.. Let me try again.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 25, 2021

Looking at the diff to upstream:

  • Is the diff in ubsan_checks.inc necessary?

These are part of the existing 10.0.0 changes. I don't really know why they were removed, but they were. Maybe something to do with the way emscripten handles NULL vs native platforms.

See the existing 10.0.0 changes:
llvm/llvm-project@llvmorg-10.0.0...emscripten-core:emscripten-libs-10.0.0#diff-8ced148882cd9876de0c3b050db5402f972d0093e2762a750177890510718c30

  • The diff in ubsan_handlers.cpp looks odd - no ifdefing for emscripten, just a pure change to upstream?

Again, these are just carried over from the current changes:

llvm/llvm-project@llvmorg-10.0.0...emscripten-core:emscripten-libs-10.0.0#diff-f899d1244fc94738aa307e798a3d7bd442539e5c97e4a93bae29dbae992195c8

We can look at cleaning them or and/removing as we look into upstreaming perhaps?

@sbc100 sbc100 merged commit fc50832 into main May 25, 2021
@sbc100 sbc100 deleted the update_compiler_rt branch May 25, 2021 16:40
sbc100 added a commit that referenced this pull request May 25, 2021
sbc100 added a commit that referenced this pull request May 25, 2021
sbc100 added a commit that referenced this pull request May 25, 2021
sbc100 added a commit that referenced this pull request May 26, 2021
@aheejin
Copy link
Member

aheejin commented Jun 2, 2021

It looks several asan/lsan tests started to break after this. Starting commit: https://ci.chromium.org/ui/p/emscripten-releases/builders/ci/linux-test-suites/b8846238787694172272/overview

@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 2, 2021

Ah yes dropped the ball on that.. thanks for reminding me.

sbc100 added a commit that referenced this pull request Jun 3, 2021
This test leaks allocates the asyncify stack during the exit handlers and
its never freed.

Interestingly enough this leak was not caught prior to #14280.
I did some investigation and found that prior to #14280 this
chunk was bring detected a reachable via the conservative stack
scan here:

 https://github.com/emscripten-core/emscripten/blob/98a8cbf5032f8d369be636a59180a59f1eb8833f/system/lib/compiler-rt/lib/lsan/lsan_common_emscripten.cpp#L141-L153

If I comment out this stack scan then this leak will be
detected even before #14280

Also, add `--profiling` to asan and lsan builds to make
error reports more useful.

See #14360
sbc100 added a commit that referenced this pull request Jun 3, 2021
The logic of this macro was inverted when the llvm-12 update to
compiler-rt (#14280).  This caused asan.test_setjmp_noleak to start
failing.

Its not clear it we want to keep this long term but restoring
it at least fixes the failing test.

See #14360
sbc100 added a commit that referenced this pull request Jun 3, 2021
This test leaks allocates the asyncify stack during the exit handlers and
its never freed.

Interestingly enough this leak was not caught prior to #14280.
I did some investigation and found that prior to #14280 this
chunk was bring detected a reachable via the conservative stack
scan here:

 https://github.com/emscripten-core/emscripten/blob/98a8cbf5032f8d369be636a59180a59f1eb8833f/system/lib/compiler-rt/lib/lsan/lsan_common_emscripten.cpp#L141-L153

If I comment out this stack scan then this leak will be
detected even before #14280

Also, add `--profiling` to asan and lsan builds to make
error reports more useful.

See #14360
sbc100 added a commit that referenced this pull request Jun 3, 2021
The logic of this macro was inverted when the llvm-12 update to
compiler-rt (#14280).  This caused asan.test_setjmp_noleak to start
failing.

Its not clear it we want to keep this long term but restoring
it at least fixes the failing test.

See #14360
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants